Repository navigation
feat: DISTINCT handling for Aggregate UDFs - #25288
Merged
Merged
Conversation
github-actions
Bot
added
documentation
mkleen
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25288 +/- ##
==========================================
- Coverage 81.94% 81.93% -0.02%
==========================================
Files 1135 1136 +1
Lines 428278 428677 +399
Branches 428278 428677 +399
==========================================
+ Hits 350947 351222 +275
- Misses 56375 56438 +63
- Partials 20956 21017 +61 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mkleen
marked this pull request as ready for review
September 14, 2026 09:04
mkleen
changed the title
Contributor
Author
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing eliminate-aggregate-distinct (ad518d5) to 7b00b63 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
jayzhan211
approved these changes
Sep 14, 2026
jayzhan211
left a comment
jayzhan211
left a comment
Contributor
There was a problem hiding this comment.
Thanks @mkleen , overall LGTM, suggestion on the naming
adriangb
approved these changes
Sep 14, 2026
adriangb
left a comment
adriangb
left a comment
Contributor
There was a problem hiding this comment.
Thank you for the new PR!
One request that is not in the inline comments:
Add unit tests with custom UDAFs for the public API. No test overrides distinct_handling. The unit tests use only the built-in min, max, sum and bit_xor, and no test references DistinctHandling::Unsupported. Please add tests in datafusion/core/tests/user_defined/user_defined_aggregates.rs (or in the rule's own tests with a test UDAF) that cover: a UDAF that returns Ignored loses the flag and keeps its output name, with FILTER and ORDER BY carried over; a UDAF that returns Honored keeps the flag; a UDAF that returns Unsupported keeps the flag; an Ignored UDAF beside an Honored distinct on the same node keeps both flags; an input that is already aliased (.alias("m")) gets no second alias; and a UDAF registered through AggregateUDF::with_aliases delegates the tag. Please also add a sqllogictest case for approx_distinct(DISTINCT v). It is the largest plan change in the PR and the one Ignored tag with no test.
The inline comments cover the rest: two test cases, the doc comments in the rule, and the definitions of the DistinctHandling variants.
Follow-up tracking issues, not for this PR:
datafusion/spark:collect_setalways usesDistinctArrayAggAccumulator, so it meets theIgnoreddefinition, andcollect_listandtry_sumnever readis_distinct. None of them is tagged.datafusion/ffi:ForeignAggregateUDFdoes not forwarddistinct_handling, so every UDAF that crosses the FFI boundary staysHonored. Safe, but the optimization cannot reachdatafusion-pythonusers.
Contributor
Author
|
@adriangb @jayzhan211 Thanks a lot for the review! I am on it with the follow-ups. |
…ag when it is a no-op
Nothing recorded whether an aggregate cares about duplicates in its input.
`min(DISTINCT x)` already built a plain `MinAccumulator`, so the flag was a
no-op at execution time, but `SingleDistinctToGroupBy` still saw it and
rewrote `SELECT g, min(DISTINCT x) FROM t GROUP BY g` into an inner group by
at `(g, x)` grain. Callers also had no way to ask the question: the rule
identified sum/min/max by lowercased string name.
Add `AggregateUDFImpl::distinct_handling`, returning a three-valued
`DistinctHandling`:
- `Ignored` - the merge is idempotent, so `DISTINCT` cannot change the
result and the planner may drop it
- `Honored` - the accumulator reads `is_distinct` and deduplicates
- `Unsupported` - the accumulator does not implement `DISTINCT`
`Honored` is the default, so external UDFs are unaffected. The property is
forwarded from both the `AggregateUDF` wrapper and `AliasedAggregateUDFImpl`.
Tag the built-ins. `Ignored`: min, max, bool_and, bool_or, approx_distinct,
and the AND/OR arms of the bitwise operation (XOR cancels duplicate pairs and
stays `Honored`). `Unsupported`: the aggregates that either reject `DISTINCT`
already or silently return the non-distinct answer. first_value, last_value
and any_value keep the default with a TODO, because whether they ignore
duplicates depends on `ORDER BY`.
Add `EliminateAggregateDistinct`, inserted immediately before
`SingleDistinctToGroupBy`. It returns `Transformed::no` for every node that is
not an `Aggregate`, and uses `NamePreserver` to keep the output schema intact,
so the plan reads `min(t.v) AS min(DISTINCT t.v)`.
`Unsupported` is declared but not yet enforced. Rejecting `corr(DISTINCT x, y)`
fixes a silent wrong answer but turns queries that are accepted today into
planning errors, which belongs in its own change.
Over 4,000,000 rows in 2,000 groups, `min(DISTINCT x)` needed a 192M pool
before this change and completes under 1M after it.
Part of apache#24929, tracked as apache#11686.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015YwFpxheiiUNMadTNyeHiu
…gged functions Review of the previous commit turned up two problems. `EliminateAggregateDistinct` stripped a no-op `DISTINCT` from one aggregate without regard for the others on the same node, which changed which plans `SingleDistinctToGroupBy` rewrites. That rule keys off how many distinct aggregates a node has and whether they share an argument, so removing one can push a node either way: `sum(DISTINCT cast(x)), max(DISTINCT x)` newly qualified once the max lost its flag, gaining an inner group by it never had, and where a rewrite happened regardless a shared inner group key turned into a separate accumulator at that finer grain. Gate the rule on every `DISTINCT` in the node being `Ignored`. This is conservative - `min(DISTINCT x), count(DISTINCT y)` now keeps the min flag though no rewrite is possible either way - but it cannot regress a plan. `group_by.slt` returns to its original expectations. `stddev`, `stddev_pop`, `var_samp`, `var_pop` and `approx_median` were tagged `Unsupported` on the grounds that their accumulators return `not_impl_err!`. That path is only reached when `SingleDistinctToGroupBy` cannot fire. All five take a single argument, so the rewrite deduplicates the input for them and `f(DISTINCT x)` returns the right answer today, as `aggregate.slt:749` already asserts for `approx_median`. Leave them at the default `Honored` with a comment, since enforcing `Unsupported` later would have broken those queries. Verified the remaining `Unsupported` tags: `approx_percentile_cont`, `nth_value` and `covar_samp` take more than one argument, so no rewrite fires and they silently return the non-distinct answer, while `approx_percentile_cont_with_weight` errors. Also widen the `Honored` and `Unsupported` doc comments, which described only the accumulator and promised a planning-time rejection that does not exist yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YwFpxheiiUNMadTNyeHiu
Clear the `DISTINCT` flag in place instead of matching the expression twice and rebuilding `AggregateFunctionParams` field by field. Collect the distinct aggregates' `DistinctHandling` and check them in one expression rather than tracking two flags with an early stop, and restore the saved name through `Transformed::update_data`. The strip helper still checks `DistinctHandling::Ignored` itself rather than relying on the node-level gate: the gate only inspects `aggr_expr`, while `map_expressions` also visits `group_expr`. A plan built with `groupBy=[sum(DISTINCT c)], aggr=[min(DISTINCT b)]` would otherwise lose the `sum` flag; SQL cannot produce it, but `Aggregate::try_new` does not reject it. `keep_honored_distinct_in_group_expr` covers this. Drop `eliminate_distinct_from_max`, which duplicated the `min` test and is covered by the min+max test. No plan changes: all optimizer snapshots and the aggregates_simplify, group_by, single_distinct_to_groupby and explain sqllogictests pass unmodified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01679fptc2vZkRV9hP4Eu5XB
The comments left on these functions in 957c8d6 said the accumulator rejects `DISTINCT` but `SingleDistinctToGroupBy` always deduplicates the input first, so `f(DISTINCT x)` returns the right answer today. Neither half holds for all five. `stddev`, `stddev_pop` and `approx_median` do reject `DISTINCT`, but they only work when the rewrite applies. Beside an aggregate that blocks it, such as `avg`, the query errors: SELECT g, stddev(DISTINCT x), avg(y) FROM t GROUP BY g Error: This feature is not implemented: STDDEV_POP(DISTINCT) aggregations are not available `var_samp` and `var_pop` never needed the rewrite: `DistinctVarianceAccumulator` deduplicates the input whenever `is_distinct` is set. Reword those comments, and the `DistinctHandling::Honored` doc, which listed `var_samp` among the planner-deduplicated functions and presented the rewrite as unconditional. Comments only; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01679fptc2vZkRV9hP4Eu5XB
…Handling non_exhaustive `grouping` was tagged `Unsupported` on the grounds that its accumulator silently returns the non-distinct answer. It does neither: the accumulator is never built, because `ResolveGroupingFunction` replaces the call with a value derived from the grouping id, and that value depends only on which grouping set a row belongs to. `grouping(DISTINCT x)` already returns the same result as `grouping(x)`, so enforcing `Unsupported` later would have rejected a working query. Tag it `Ignored`. The UDAF guide said to return `Unsupported` whenever the accumulator does not implement deduplication, which would cover `stddev` and `approx_median`, both left at `Honored` because `SingleDistinctToGroupBy` deduplicates for them when it applies. State the full condition, matching the enum doc, and say where those functions belong. `DistinctHandling` is public and nothing outside the defining crate matches on it exhaustively, so mark it `#[non_exhaustive]` now, before a release makes adding a variant a breaking change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01679fptc2vZkRV9hP4Eu5XB
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
The rule's tests read the tag off built-in functions, so they only covered the variants those functions carry, and nothing exercised an `AggregateUDFImpl::distinct_handling` override, the API a third-party function uses. Add a `TaggedUdaf` test helper that reports the handling it was built with, and tests that an `Ignored` UDAF loses the flag while keeping its output name, `FILTER` and `ORDER BY`; that `Honored` and `Unsupported` keep it; that an `Ignored` UDAF beside an `Honored` one keeps both; that an aggregate the caller already aliased gets no second alias; and that `AggregateUDF::with_aliases` passes the tag through. Also add a sqllogictest for `approx_distinct(DISTINCT v)`, the largest plan change the rule makes and the one `Ignored` tag with no test: the deduplicating inner group by that `SingleDistinctToGroupBy` planned here is gone, because setting an HLL register a value already reached is a no-op. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sitive The old names described what the planner does with the flag, which reads backwards at the call site: a function returning `Ignored` is the one whose `DISTINCT` can be dropped, not one that ignores the modifier's meaning. The new names say what the function is: insensitive or sensitive to duplicates in its input. `Unsupported` is unchanged. Identifiers and prose that echoed the old names follow, including `strip_ignored_distinct` and four test names in the rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mkleen
force-pushed
the
eliminate-aggregate-distinct
branch
from
September 16, 2026 09:44
616f613 to
9e96779
Compare
This was referenced Sep 16, 2026
adriangb
approved these changes
Sep 16, 2026
pull Bot
pushed a commit
to Stars1233/datafusion
that referenced
this pull request
Sep 19, 2026
## Which issue does this PR close? - N/A; see apache#22652 and apache#25288 for context ## Rationale for this change If we can prove that parts of a query are insensitive to duplicates, the optimizer can apply various simplifications, like replacing inner joins with semi-joins and removing unused outer-join inputs. The join analysis was previously conservative and assumed that all aggregate expressions are duplicate sensitive. Since apache#25288 added a framework for classifying how an aggregate treats duplicate values, we can now apply that framework to optimize joins more effectively. ## What changes are included in this PR? - Extend `EliminateJoin` to recognize `Aggregate` plan nodes whose aggregate expressions are all either `DistinctHandling::Insensitive` or `DistinctHandling::Sensitive` and invoked with `DISTINCT` - Guard duplicate-insensitivity propagation against volatile expressions and subqueries, to avoid changing query results - Refactor code to share the existing volatility/subquery check with UnionsToFilter. - Add tests ## What is the testing strategy for this PR? Existing tests pass; new tests added. No TPC-H plans change. ## Are there any user-facing changes? Some query plans might change (usually for the better).
Omega359
pushed a commit
to Omega359/arrow-datafusion
that referenced
this pull request
Oct 11, 2026
## Which issue does this PR close? - Closes apache#22644. - Closes apache#11686 - Part of apache#24929 (the `min`/`max` case). ## Rationale for this change `SELECT g, min(DISTINCT x) FROM t GROUP BY g` is planned as if `DISTINCT` mattered. `SingleDistinctToGroupBy` rewrites it into an inner group by at `(g, x)` grain just to deduplicate `x`, even though `min` returns the same value with or without duplicates. On a 4M-row table with ~1M distinct `x` values, that extra stage makes the query **up to 10x slower** and reserves **~190 MB** where the plain `min(x)` needs less than 1 MB. The same applies to every aggregate whose result cannot change when duplicates are removed: `max`, `bool_and`, `bool_or`, `bit_and`, `bit_or`, `approx_distinct`. Nothing in DataFusion could say which aggregates those are. `SingleDistinctToGroupBy` identifies `sum`/`min`/`max` by lowercased function name, and a user-defined aggregate had no way to declare the property at all. ## What changes are included in this PR? **A new `AggregateUDFImpl::distinct_handling()` method** returning a new enum `DistinctHandling`: | Variant | Meaning | Built-ins | |---|---|---| | `Insensitive` | Duplicates cannot change the result (the merge is idempotent), so the planner may drop `DISTINCT`. | `min`, `max`, `bool_and`, `bool_or`, `bit_and`, `bit_or`, `approx_distinct`, `grouping` | | `Sensitive` *(default)* | `DISTINCT` is applied, either by the accumulator itself (`count`, `sum`, `avg`, `var_samp`, `array_agg`, `bit_xor`, ...) or by `SingleDistinctToGroupBy` deduplicating the input first (`stddev`, `approx_median`). | everything not listed | | `Unsupported` | The result depends on duplicates, but nothing deduplicates the input, so `f(DISTINCT ...)` errors or silently returns the non-distinct answer. Declaration only for now; rejecting these at planning time is a follow-up. | `corr`, `covar_samp`, `covar_pop`, `regr_*`, `nth_value`, `approx_percentile_cont`, `approx_percentile_cont_with_weight` | `first_value`, `last_value` and `any_value` stay at `Sensitive` with a TODO: whether `DISTINCT` is a no-op for them depends on `ORDER BY`. **A new logical optimizer rule, `EliminateAggregateDistinct`**, placed before `SingleDistinctToGroupBy`. It clears the `DISTINCT` flag on `Insensitive` aggregates and keeps the output column name with an alias: ```text Aggregate: groupBy=[[g]], aggr=[[min(DISTINCT x)]] -- becomes Aggregate: groupBy=[[g]], aggr=[[min(x) AS min(DISTINCT x)]] ``` ## What is the testing strategy for this PR? - **Unit tests** in `datafusion/optimizer/src/eliminate_aggregate_distinct.rs`: `min`, `min`+`max`, a non-distinct aggregate beside the distinct one, `FILTER`, the negative cases `sum` and `bit_xor`, a mixed node that must be left alone (on its own and followed by `SingleDistinctToGroupBy`), an `Sensitive` distinct aggregate in the group expressions, and a plan with no `Aggregate`. - **sqllogictest:** - `aggregates_simplify.slt` gets a new section. For each `Ignored` aggregate it checks the `EXPLAIN` plan and the query results, over data with duplicates and `NULL`s. It also covers the negative cases (`bit_xor`, `count`), a mixed node, and `FILTER`. - `single_distinct_to_groupby.slt` has an updated expectation for `min(DISTINCT v)`. - `explain.slt` lists the new rule. - **Documentation:** a new "Declaring how an Aggregate UDF treats `DISTINCT`" section in `adding-udfs.md`, and the rule in `optimizer_rule_reference.md`. ### Benchmark Here are some results from a local benchmark. Every query has the shape `SELECT g, <aggregate> FROM t GROUP BY g`, over 4M rows where `x` has about 1M distinct values and `b` has two. Speedup (`main` time / branch time): | query | 2k groups, 1 part | 2k groups, 10 parts | 500k groups, 1 part | 500k groups, 10 parts | |---|---:|---:|---:|---:| | min | 10.10x | 6.56x | 3.44x | 2.49x | | max | 10.37x | 7.19x | 3.35x | 2.44x | | bit_and | 10.36x | 7.73x | 3.50x | 2.53x | | bit_or | 10.25x | 7.10x | 3.33x | 2.52x | | bool_and | 1.66x | 1.53x | 1.51x | 2.36x | | bool_or | 1.50x | 1.65x | 1.50x | 1.71x | | approx_distinct | 1.97x | 1.77x | 1.77x | 1.77x | Peak memory-pool reservation in MB (`main` → branch): | query | 2k groups, 1 part | 2k groups, 10 parts | 500k groups, 1 part | 500k groups, 10 parts | |---|---:|---:|---:|---:| | min | 191.4 → 0.2 | 167.9 → 1.5 | 191.4 → 24.0 | 167.9 → 42.7 | | max | 191.4 → 0.2 | 167.9 → 1.5 | 191.4 → 24.0 | 167.8 → 42.9 | | bit_and | 191.4 → 0.2 | 168.2 → 1.5 | 191.4 → 24.0 | 167.5 → 41.8 | | bit_or | 191.4 → 0.2 | 167.6 → 1.5 | 191.4 → 24.0 | 167.5 → 42.0 | | bool_and | 0.3 → 0.1 | 2.7 → 1.3 | 28.2 → 20.0 | 34.1 → 26.0 | | bool_or | 0.3 → 0.1 | 2.7 → 1.3 | 28.2 → 20.0 | 35.4 → 26.9 | | approx_distinct | 191.4 → 31.5 | 170.5 → 41.5 | 191.4 → 62.5 | 168.0 → 88.5 | ## Are there any user-facing changes? - `AggregateUDFImpl::distinct_handling()` with default `DistinctHandling::Sensitive`, so existing implementations compile and behave as before. ## LLM-generated code disclosure This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Omega359
pushed a commit
to Omega359/arrow-datafusion
that referenced
this pull request
Oct 11, 2026
## Which issue does this PR close? - N/A; see apache#22652 and apache#25288 for context ## Rationale for this change If we can prove that parts of a query are insensitive to duplicates, the optimizer can apply various simplifications, like replacing inner joins with semi-joins and removing unused outer-join inputs. The join analysis was previously conservative and assumed that all aggregate expressions are duplicate sensitive. Since apache#25288 added a framework for classifying how an aggregate treats duplicate values, we can now apply that framework to optimize joins more effectively. ## What changes are included in this PR? - Extend `EliminateJoin` to recognize `Aggregate` plan nodes whose aggregate expressions are all either `DistinctHandling::Insensitive` or `DistinctHandling::Sensitive` and invoked with `DISTINCT` - Guard duplicate-insensitivity propagation against volatile expressions and subqueries, to avoid changing query results - Refactor code to share the existing volatility/subquery check with UnionsToFilter. - Add tests ## What is the testing strategy for this PR? Existing tests pass; new tests added. No TPC-H plans change. ## Are there any user-facing changes? Some query plans might change (usually for the better).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
min/maxcase).Rationale for this change
SELECT g, min(DISTINCT x) FROM t GROUP BY gis planned as ifDISTINCTmattered.SingleDistinctToGroupByrewrites it into an inner group by at(g, x)grain just to deduplicatex, even thoughminreturns the same value with or without duplicates. On a 4M-row table with ~1M distinctxvalues, that extra stage makes the query up to 10x slower and reserves ~190 MB where the plainmin(x)needs less than 1 MB.The same applies to every aggregate whose result cannot change when duplicates are removed:
max,bool_and,bool_or,bit_and,bit_or,approx_distinct.Nothing in DataFusion could say which aggregates those are.
SingleDistinctToGroupByidentifiessum/min/maxby lowercased function name, and a user-defined aggregate had no way to declare the property at all.What changes are included in this PR?
A new
AggregateUDFImpl::distinct_handling()method returning a new enumDistinctHandling:InsensitiveDISTINCT.min,max,bool_and,bool_or,bit_and,bit_or,approx_distinct,groupingSensitive(default)DISTINCTis applied, either by the accumulator itself (count,sum,avg,var_samp,array_agg,bit_xor, ...) or bySingleDistinctToGroupBydeduplicating the input first (stddev,approx_median).Unsupportedf(DISTINCT ...)errors or silently returns the non-distinct answer. Declaration only for now; rejecting these at planning time is a follow-up.corr,covar_samp,covar_pop,regr_*,nth_value,approx_percentile_cont,approx_percentile_cont_with_weightfirst_value,last_valueandany_valuestay atSensitivewith a TODO: whetherDISTINCTis a no-op for them depends onORDER BY.A new logical optimizer rule,
EliminateAggregateDistinct, placed beforeSingleDistinctToGroupBy. It clears theDISTINCTflag onInsensitiveaggregates and keeps the output column name with an alias:What is the testing strategy for this PR?
datafusion/optimizer/src/eliminate_aggregate_distinct.rs:min,min+max, a non-distinct aggregate beside the distinct one,FILTER, the negative casessumandbit_xor, a mixed node that must be left alone (on its own and followed bySingleDistinctToGroupBy), anSensitivedistinct aggregate in the group expressions, and a plan with noAggregate.aggregates_simplify.sltgets a new section. For eachIgnoredaggregate it checks theEXPLAINplan and the query results, over data with duplicates andNULLs. It also covers the negative cases (bit_xor,count), a mixed node, andFILTER.single_distinct_to_groupby.slthas an updated expectation formin(DISTINCT v).explain.sltlists the new rule.DISTINCT" section inadding-udfs.md, and the rule inoptimizer_rule_reference.md.Benchmark
Here are some results from a local benchmark. Every query has the shape
SELECT g, <aggregate> FROM t GROUP BY g, over 4M rows wherexhas about 1M distinct values andbhas two.Speedup (
maintime / branch time):Peak memory-pool reservation in MB (
main→ branch):Are there any user-facing changes?
AggregateUDFImpl::distinct_handling()with defaultDistinctHandling::Sensitive, so existing implementations compile and behave as before.LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.